Improve browser selector stability - #123
Conversation
|
|
Greptile SummaryThis PR rewrites the selector-generation logic in both browser scripts to prefer explicit test hooks (
Confidence Score: 4/5Safe to merge; the new selector logic is well-tested and the generated-ID guards work correctly in all covered paths. The selector priority rewrite is correct and well-covered by new tests. The only notable gap is that Both
|
| Filename | Overview |
|---|---|
| src/scripts/get-browser-accessibility-tree.ts | Adds getSelectorAccessibleName, isLikelyGeneratedId, and uniqueCssSelector helpers; rewrites getSelector to prioritise test-hook attributes, then aria/ name, then stable id/name/placeholder/class, then CSS path. The CSS path loop now guards generated IDs correctly. Minor: getSelectorAccessibleName omits the title fallback that getAccessibleName uses, so title-only named elements get a CSS selector even though aria/title-text would work. |
| src/scripts/get-interactable-browser-elements.ts | Adds getRole, getSelectorAccessibleName, isLikelyGeneratedId, and uniqueCssSelector helpers; rewrites getSelector with the same priority order as the accessibility-tree script. Also skips generated IDs in the CSS path loop. Contains the same title fallback omission as the accessibility-tree script. |
| tests/scripts/accessibility-tree.test.ts | Adds tests for test-hook priority, data-test/data-qa hooks, generated-ID skipping (both early-return and CSS-path fallback), and placeholder selectors. Correctly updates assertions from tag*=text to aria/name. |
| tests/scripts/interactable-elements.test.ts | Mirrors the accessibility-tree test additions: test-hook priority, aria/name fallback, generated-ID skipping, CSS-path fallback, and placeholder selector. All assertions look correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getSelector called] --> B{data-testid / data-test / data-qa unique?}
B -- yes --> Z1["return [attr=value]"]
B -- no --> C{role + accessible name unique?}
C -- yes --> Z2["return aria/name"]
C -- no --> D{element.id and NOT generated?}
D -- yes --> Z3["return #id"]
D -- no --> E{"[name] unique?"}
E -- yes --> Z4["return tag[name=val]"]
E -- no --> F{"[placeholder] unique?"}
F -- yes --> Z5["return tag[placeholder=val]"]
F -- no --> G{unique class selector?}
G -- yes --> Z6["return tag.class"]
G -- no --> H[CSS path walk ≤ 4 levels]
H --> I{ancestor.id and NOT generated?}
I -- yes --> Z7["return #ancestor-id > … > tag"]
I -- no --> Z8["return structural path"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[getSelector called] --> B{data-testid / data-test / data-qa unique?}
B -- yes --> Z1["return [attr=value]"]
B -- no --> C{role + accessible name unique?}
C -- yes --> Z2["return aria/name"]
C -- no --> D{element.id and NOT generated?}
D -- yes --> Z3["return #id"]
D -- no --> E{"[name] unique?"}
E -- yes --> Z4["return tag[name=val]"]
E -- no --> F{"[placeholder] unique?"}
F -- yes --> Z5["return tag[placeholder=val]"]
F -- no --> G{unique class selector?}
G -- yes --> Z6["return tag.class"]
G -- no --> H[CSS path walk ≤ 4 levels]
H --> I{ancestor.id and NOT generated?}
I -- yes --> Z7["return #ancestor-id > … > tag"]
I -- no --> Z8["return structural path"]
Reviews (2): Last reviewed commit: "Improve browser selector stability" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (role && accessibleName && accessibleName.length <= 80) { | ||
| let matchCount = 0; | ||
| document.querySelectorAll('*').forEach(el => { | ||
| const htmlEl = el as HTMLElement; | ||
| const candidateRole = getRole(htmlEl); | ||
| if (isVisible(htmlEl) && candidateRole && getSelectorAccessibleName(htmlEl, candidateRole) === accessibleName) matchCount++; | ||
| }); | ||
| if (matchCount === 1) return `aria/${accessibleName}`; | ||
| } |
There was a problem hiding this comment.
querySelectorAll('*') scans the entire DOM on every aria/ uniqueness check
The interactable-elements script narrows the scan to interactableSelectors, but the accessibility-tree script calls document.querySelectorAll('*').forEach(...) for every element that has a role and accessible name. On a real page with thousands of DOM nodes — and with getSelector being called once per accessibility node — this is an O(n²) walk that can noticeably stall browser.execute() on content-heavy pages. Consider narrowing to a role-bearing subset rather than scanning every element in the document.
544e044 to
43f9417
Compare
| const accessibleName = getSelectorAccessibleName(element, role); | ||
| if (role && accessibleName && accessibleName.length <= 80) { | ||
| let matchCount = 0; | ||
| document.querySelectorAll('*').forEach(el => { |
There was a problem hiding this comment.
issue: querySelectorAll('*') walks the entire DOM for every aria/ uniqueness check -> O(n²)
Each invocation of getSelector() for an element with a viable accessible name scans all DOM elements (line 190), calling getRole(), isVisible(), and getSelectorAccessibleName() on every one. On a 10K-element page with 100 elements hitting this branch, that's 1M iterations, each non-trivial.
The interactable-elements script (line 202) correctly scopes this to interactableSelectors. At minimum, this should scope to elements that the walker actually assigns roles to — e.g. '[role], [aria-label], [aria-labelledby], a[href], button, input, select, textarea, h1-h6, img, nav, main, header, footer, aside, dialog, form, section'.
Ideally, precompute role + accessibleName → count in a single pass during the tree walk and do O(1) lookups here instead.
| return (el.textContent?.trim().replace(/\s+/g, ' ') || '').slice(0, 100); | ||
| } | ||
|
|
||
| function getSelectorAccessibleName(el: HTMLElement, role: string | null): string { |
There was a problem hiding this comment.
suggestion: getAccessibleName and getSelectorAccessibleName are near-duplicates with subtle behavioral differences
The only differences: getSelectorAccessibleName omits placeholder and title fallbacks and adds a CONTAINER_ROLES guard. These omissions are correct for selector generation — placeholder and title produce brittle selectors — but the intent is obscured by having two 30-line functions that are ~80% identical.
If someone patches getAccessibleName in the future without realizing getSelectorAccessibleName exists, selector names will silently diverge from the reported name field.
| return null; | ||
| } | ||
|
|
||
| function getSelectorAccessibleName(el: HTMLElement): string { |
There was a problem hiding this comment.
suggestion: Same getAccessibleName / getSelectorAccessibleName duplication as the accessibility-tree script
getAccessibleName (L60–109) covers 8 fallback layers. getSelectorAccessibleName (L137–172) stops at textContent, skipping placeholder and title. The behavioral split is intentional (placeholder/title selectors are unstable), but the structural duplication carries the same maintenance risk as in the sibling script.
Same suggestion: rename to getStableSelectorName() with a comment explaining the deliberate omission.
2 participants
Summary
data-testid,data-test,data-qa) before accessible-name selectorsVerification
pnpm install --frozen-lockfilepnpm exec tsc --noEmitpnpm testNotes
pnpm auditreports existing dependency advisories unrelated to this selector change